Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better track ForceBalance Failures #217

Merged
merged 2 commits into from
Jan 19, 2023
Merged

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Jan 18, 2023

Description

This implements @jthorton's suggested fix: #216 (comment)

I don't know how to test for this or if any tests are failing on the upstream branch.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • TODO 1

Questions

  • Question1

Status

  • Ready to go

@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #217 (26fc213) into 0-1-x (3abdcca) will decrease coverage by 0.08%.
The diff coverage is 57.14%.

Additional details and impacted files

@mattwthompson
Copy link
Member Author

Worth noting that the most recent version of ForceBalance is pulled down

$ wget -O - https://pipelines.actions.githubusercontent.com/serviceHosts/415507c3-0cf5-4512-8746-f9c7b4b67c91/_apis/pipelines/1/runs/2171/signedlogcontent/2\?urlExpires\=2023-01-18T21%3A37%3A43.5258060Z\&urlSigningMethod\=HMACV1\&urlSignature\=GghPzvoDHiYWXVMLR8NPtCXYDaY9KOz63foLKfWIomQ%3D  | grep "forcebalance.*conda-forge"
--2023-01-18 15:37:02--  https://pipelines.actions.githubusercontent.com/serviceHosts/415507c3-0cf5-4512-8746-f9c7b4b67c91/_apis/pipelines/1/runs/2171/signedlogcontent/2?urlExpires=2023-01-18T21%3A37%3A43.5258060Z&urlSigningMethod=HMACV1&urlSignature=GghPzvoDHiYWXVMLR8NPtCXYDaY9KOz63foLKfWIomQ%3D
Resolving pipelines.actions.githubusercontent.com (pipelines.actions.githubusercontent.com)... 13.107.42.16
Connecting to pipelines.actions.githubusercontent.com (pipelines.actions.githubusercontent.com)|13.107.42.16|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: unspecified [text/plain]
Saving to: ‘STDOUT’

-                                         [<=>                                                                       ]       0  --.-KB/s               2023-01-18T21:21:00.6596784Z   + forcebalance                          1.9.4  py38h507a481_0          conda-forge/linux-64     628kB
2023-01-18T21:22:07.9795057Z   forcebalance                   1.9.4         py38h507a481_0          conda-forge
2023-01-18T21:22:18.9081378Z forcebalance              1.9.4            py38h507a481_0    conda-forge
-                                         [ <=>                                                                      ] 238.83K  --.-KB/s    in 0.09s

2023-01-18 15:37:03 (2.74 MB/s) - written to stdout [244567]

Copy link
Contributor

@Yoshanuikabundi Yoshanuikabundi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I think this is the right thing to do for 0.1.4. We should also update the builds of 0.1.x to require forcebalance < 1.9.4 if possible, as pinning the toolkit to <0.11 doesn't seem to be cutting it.

I don't think this is really possible to test in any meaningful way without full integration testing, but if you wanted to create an optimise.err in the appropriate place and then call this function in a unit test, we could do that. I don't think it's much of a win though - I think the major way this would break is if the name of the error file changed, which isn't local to this function. I'll create an "integration tests!" issue and make sure this is on the list of things to integrationally test.

@mattwthompson mattwthompson merged commit e01d7de into 0-1-x Jan 19, 2023
@Yoshanuikabundi Yoshanuikabundi mentioned this pull request Jan 19, 2023
8 tasks
@mattwthompson mattwthompson deleted the capture-forcebalance-error branch January 25, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants